Fix partial/complete data values for incremental responses, especially when combined with returnPartialData - #13324
Conversation
| data: { | ||
| friendList: [ | ||
| { __typename: "Friend", id: "1", name: "Luke" }, | ||
| { __typename: "Friend", id: "2" }, | ||
| { __typename: "Friend", id: "3" }, | ||
| ], | ||
| }, | ||
| dataState: "partial", |
There was a problem hiding this comment.
This is the other major change in this PR. Partial list items were truncated after the first chunk, even though returnPartialData: true was set. This was a mistake and could cause janky UX if list items suddenly disappear in the UI as the list streams in.
The partial list items are now retained and dataState properly set to partial. Once there are no more partial items, dataState changes to complete instead of streaming since the data fully satisfies the query and it is safe to access all fields.
c7dec14 to
f621b2b
Compare
|
I added a failing test case in f1764f1 which will need to be fixed in 4.3 before we release, however I think its worth getting this out in a new alpha now and following up in a separate PR. I opened issue #13330 to ensure we captured the case.
|
…cremental results (#13333) While implementing the incremental fixes in #13324, I noticed another case we aren't handling correctly with incremental streaming: `read` functions are run, but the results are never applied to intermediate incremental results. `cache.diff` runs the `read` functions, but because we only apply `diff.result` when `diff.complete` (which is `false` when `@defer` chunks haven't streamed yet), we never see the transformed values. I want to make sure #13324 fixes most (if not all) of these cases, so I'm creating a separate test suite for this. These tests are added in a separate file to avoid conflicts with the test file changes from #13324. That PR will be remove this test file and copy over the tests to maintain a clean merge. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Added comprehensive coverage for incremental GraphQL delivery using `@defer` and `@stream`. * Validated cache field transformations across partial results, streamed list items, nested objects, overlapping selections, and residual cache data. * Documented expected behavior for scenarios still under development. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
870aa29 to
14498bd
Compare
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to release-4.3, this PR will be updated.⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ `release-4.3` is currently in **pre mode** so this branch has prereleases rather than normal releases. If you want to exit prereleases, run `changeset pre exit` on `release-4.3`.⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ # Releases ## @apollo/client@4.3.0-alpha.3 ### Minor Changes - [#13324](#13324) [`0abd8de`](0abd8de) Thanks [@jerelmiller](https://github.com/jerelmiller)! - Fix the accuracy of `dataState` in complex incremental streaming scenarios, especially when combined with `returnPartialData: true`. Prior to this change, all intermediate chunks used for both `@defer` and `@stream` directives returned a `dataState` of `streaming`, regardless of whether the actual data shape fit the definition of the `streaming` data state. The `streaming` data state represents an incomplete incremental response where the only holes in the data occur at `@defer` boundaries. Let's use the following example of where the previous `dataState` fell down when combined with `returnPartialData`. ```gql query GreetingQuery { greeting { message ... @defer { recipient { name email } } } } ``` 1. Scenario 1: partial data inside a `@defer` boundary written to the cache Let's say the cache contained the following partial data: ```ts { greeting: { __typename: "Greeting", recipient: { __typename: "Person", name: "John Doe", }, }, }; ``` After the first chunk arrives from the server, the data looks like the following: ```ts { greeting: { __typename: "Greeting", message: "Hello, John", recipient: { __typename: "Person", name: "John Doe", }, }, }; ``` This data is not `complete` because `recipient.email` is missing. This data is also not `streaming` because the data requirements in the `@defer` boundary are partially fulfilled due to the existence of `recipient`. This could lead to runtime crashes on `recipient.email` if you use the existence of `recipient` to detect whether data in the `@defer` boundary has streamed in or not. This change now accurately reports this as `partial` to ensure the field is marked as a partial field in `recipient`. 2. Scenario 2: partial data written to the cache that fulfills the data requirements of the `@defer` boundary Let's say the cache contained the following partial data: ```ts { greeting: { __typename: "Greeting", recipient: { __typename: "Person", name: "John Doe", email: "john@example.com", }, }, }; ``` After the first chunk arrives from the server, the data looks like the following: ```ts { greeting: { __typename: "Greeting", message: "Hello, John", recipient: { __typename: "Person", name: "John Doe", email: "john@example.com", }, }, }; ``` In this case, the combination of the first chunk and the partial data in the cache now fulfills the data requirements of the query. Even though the server is still streaming data (`NetworkStatus.streaming`), we can report this as `dataState: "complete"` since it is safe to access data on all fields. This change also means `@stream` queries by definition fulfill the data requirements of the query after the first chunk arrives since `@stream` operates on lists and contains no data holes. `@stream` queries now accurately report `dataState` as `complete` or `partial`, depending on whether the list mixes partial data with streamed list items. As a result of this change, some cases where you'd previously see `dataState` reported as `"streaming"` are now reported as `partial` or `complete`. If you use `dataState` to determine whether an incremental request is still in-flight, please use `networkStatus` instead to check for `NetworkStatus.streaming`. `dataState` is type narrowing feature and not intended to report the network status. ### Patch Changes - [#13324](#13324) [`0abd8de`](0abd8de) Thanks [@jerelmiller](https://github.com/jerelmiller)! - Fix an issue where field `read` functions were not applied to intermediate results while streaming `@defer` responses. `cache.diff` ran the `read` functions, but the transformed values were only applied to the emitted result when the updated cache result was considered complete. Intermediate chunks whose only holes were at `@defer` boundaries now correctly return the result of field `read` functions. ```ts new InMemoryCache({ typePolicies: { Greeting: { fields: { message: { read: (message) => message.toUpperCase(), }, }, }, }, }); // query GreetingQuery { // greeting { // message // ... @defer { // recipient { name } // } // } // } // First chunk previously returned: // { greeting: { message: "Hello world" } } // // Now correctly returns while still streaming: // { greeting: { message: "HELLO WORLD" } } ``` - [#13324](#13324) [`0abd8de`](0abd8de) Thanks [@jerelmiller](https://github.com/jerelmiller)! - Fix an issue with `@stream` queries when using `returnPartialData: true` where the streamed list was truncated after the first incremental chunk when the list contained partial cache data. The list is no longer truncated and partial list items are now retained as incremental chunks arrive. The `dataState` is now reported as `partial` until the server has streamed enough of the list so that each list item fully satisfies the query. This change also updates `@stream` queries so that they reported with `dataState: "complete` instead of `"streaming"` since it is safe to access all fields in the response. Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…daries during intermediate results (#13347) Fixes #13330 The big change in this PR is making `InMemoryCache` incremental-aware for cache reads (though hidden behind an internal symbol that enables it). `cache.diff` is too naive and classifies in-progress queries with `@defer` boundaries as partial when `@defer` data hasn't streamed in. This is a problem for how `QueryInfo` rereads data after a cache write because it won't apply that data to the returned result unless its fully complete. Field values transformed by cache read functions were therefore not applied to intermediate results in a `@defer` query. This issue became glaringly apparent with the introduction of custom scalars in 4.3 (currently unreleased) where we need to apply the parsed field values stored in the cache to intermediate chunks. Without that, the user might encounter runtime crashes when expecting the value to be the parsed type. #13324 was a first attempt at solving this issue across many cases where read functions/custom scalars weren't applied, but it did not fix the issue where partial data inside a `@defer` or `@stream` boundary leaked in intermediate results. It relied on the `missing` tree to reapply field values from the reread `cache.diff`, but did not prune partial boundaries. `readFromStore` can now properly handle partial boundaries when provided with `returnPartialData: true` (though only when the hidden flag is provided to ensure backwards compatibility until v5). The complex scenarios where partial data can show up inside defer boundaries (especially with overlapping selections in `@defer` boundaries) is now properly handled during a cache read. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Prevented incomplete cached data from leaking into `network-only` incremental query results. * Improved handling of partial `@defer` content and incomplete `@stream` array items. * Corrected cache result reuse when registered fragments resolve differently across queries. * **New Features** * Added richer incremental result states: complete, streaming, partial, and empty. * Improved tracking of pending deferred and streamed results. * Expanded cache memory metrics for incremental processing. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: jerelmiller <565661+jerelmiller@users.noreply.github.com>
Apollo Client 4.0-4.2.x is fairly naive in the way
dataStateis reported for incremental responses. Those versions setdataStateasstreamingif the request is still in-flight, regardless of whether the data actually satisfies the definition ofstreaming.streamingis meant to represent a state where the only holes indataare at@deferboundaries that aren't streamed in. Prior to this change, this could lead to runtime crashes due to the inaccuracy (see the changeset for an example).The other issue is that field
readfunctions that modified values on individual fields were not applied correctly in intermediate responses returned by the cache. Thereadfunctions were run correctly, but due to the completeness checks inQueryInfo, the values were never applied.datareturned in incremental chunks are now properly reported aspartial, or in some casescompletedepending on whether it fulfills the requirements of the query and/or@deferboundaries. This change also fixes the issue where fieldreadfunction return values were not applied to intermediate chunks.Summary by CodeRabbit
Bug Fixes
dataStatereporting for incremental@deferand@streamresponses.partial,streaming, andcompletebased on available cached data.New Features